Skip to content

Render some username and password in creds fixture#1949

Merged
yxieca merged 1 commit intosonic-net:masterfrom
wangxin:creds-rendering-pr
Jul 23, 2020
Merged

Render some username and password in creds fixture#1949
yxieca merged 1 commit intosonic-net:masterfrom
wangxin:creds-rendering-pr

Conversation

@wangxin
Copy link
Collaborator

@wangxin wangxin commented Jul 22, 2020

Description of PR

Summary:
Fixes # (issue)

Some username and password retrieved using the creds fixture
may have reference to other variables. If the username and
password are not consumed by ansible, they would not be
rendered. This fix renders some of the username and password
in the creds fixture.

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Approach

What is the motivation for this PR?

The ptf testing in advanced reboot failed because the dut_username and dut_password contain un-rendered variables.
This PR is to fix the issue.
The test_wr_arp.py also need to be fixed.

How did you do it?

Render the sonicadmin_user and sonicadmin_password variables using Jinja2 template and gathered vars of the duthost in the creds fixture.

How did you verify/test it?

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

@wangxin wangxin requested a review from a team July 22, 2020 09:21
@wangxin
Copy link
Collaborator Author

wangxin commented Jul 22, 2020

I think yes, it can be a shared function or method. Currently creds is a fixture. The creds fixture just retrieves all the pre-defined credentials in scope from ansible group and host var files. Rendering the credentials need input of variables specific to some host. It could be ptfhost, duthost or others. So, just enhancing the creds fixture may not be a good option. We can make it shared if we need this somewhere else in the future. By then we can have better understanding of where is the better place to add the shared function.

@bingwang-ms
Copy link
Collaborator

How about add another fixture to retrive sonic_admin_user and sonic_admin_password? I think this feature is needed by a number of cases. And I guess others may make the same mistake when use creds to get username and password.

@neethajohn
Copy link
Contributor

What about the test_wr_arp.py? Is it rendered there correctly?

Copy link
Collaborator

@yxieca yxieca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move on with this change first. Then improve later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can the template be done once in the creds fixture itself?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe fixture can be a class. But let's move on to fix the issue then improve.

@wangxin
Copy link
Collaborator Author

wangxin commented Jul 23, 2020

What about the test_wr_arp.py? Is it rendered there correctly?

Thanks for bringing this up. Unfortunately, test_wr_arp.py needs update as well. It looks like there is a need to make it shared now. I think creds fixture is not the right place to add the Template rendering code. Such username/password are specific to different type of hosts. I'll add the code to the SonicHost class.
Please don't merge this PR for now. More updates are coming soon.

@wangxin wangxin marked this pull request as draft July 23, 2020 02:58
Some username and password retrieved using the creds fixture
may have reference to other variables. If the username and
password are not consumed by ansible, they would not be
rendered. This fix renders some of the username and password
in the creds fixture.

Signed-off-by: Xin Wang <xiwang5@microsoft.com>
@wangxin wangxin changed the title Render the creds befor passing to ptf_runner Render some username and password in creds fixture Jul 23, 2020
@wangxin
Copy link
Collaborator Author

wangxin commented Jul 23, 2020

Multiple places referred to creds['sonicadmin_user'] and creds['sonicadmin_password']. It turns out rendering these two variables in the creds fixture makes more sense. I have updated this PR in that way. Thanks for all the good suggestions!

@wangxin wangxin marked this pull request as ready for review July 23, 2020 13:14
@yxieca yxieca merged commit 04de7b1 into sonic-net:master Jul 23, 2020
@wangxin wangxin deleted the creds-rendering-pr branch September 24, 2020 02:33
kazinator-arista pushed a commit to kazinator-arista/sonic-mgmt that referenced this pull request Mar 4, 2026
f81043b1f9ff02196629655f4735b33afd7f0ae1 (HEAD -> 202111, origin/202111) [port2alias]: Fix to get right number of return values (sonic-net#1906)
bbbf65943ec46e9330eadaed8bcdf1612cb8bd55 [CLI][show bgp] On chassis don't show internal BGP sessions by default (sonic-net#1927)
e12de7e7bf6cff3ec127f261bf88e4d29776d27b [port] Fix port speed set (sonic-net#1952)
cae7af752d484956d7fe40e4c3a849ddad460976 Fix invalid output of syslog IPv6 servers (sonic-net#1933)
6009341ddf790094166be5f0a81b4c114f00220b Routed subinterface enhancements (sonic-net#1821)
6ab9d67ca6550c592b97afb513804be474f84eb0 Enhance sfputil for CMIS QSFP (sonic-net#1949)
76cc67ba4f81c69b20efb3341808037c9db8f703 [debug dump] Refactoring Modules and Unit Tests (sonic-net#1943)
cff58a8171423e4012bc8caf9748996a1e98b7e2 Add command reference for trap flow counters (sonic-net#1876)
71cf3ee43524d56ad57dd90b937cfbf4bf63ba6a [Reclaim buffer] [Mellanox] Db migrator support reclaiming reserved buffer for unused ports (sonic-net#1822)
e699b49fb722e6d6fe5a1d2dacd2d39eb085c1e4 Add show command for BFD sessions (sonic-net#1942)
bb6c5774c843dbfad5f1ba00ee76dae7720902d1 [warm-reboot] Fix failures of warm reboot on disconnect of ssh session (sonic-net#1529)
2e8bbb308477862a76d2327fcf696875e8f08650 Add trap flow counter support (sonic-net#1868)
58407c1386ef13772a9a9320a795e380f162ab2c [load_minigraph] Delay pfcwd start until the buffer templates are rendered (sonic-net#1937)
eb388e0584ba1fe8d8dba58f1c5a148036ffe047 [sonic-package-manager] support sonic-cli-gen and packages with YANG model (sonic-net#1650)
2371d84e7d281bdb9988b5a1a012498dbbfb89ec generic_config_updater: Filename changed & VLAN validator added (sonic-net#1919)
7c0718dfaf23289d4ecc3ada9332e465c9a4e56b [config reload] Update command reference (sonic-net#1941)

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants